Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not set CrossBuild to true when building runtime tests for Android #55641

Merged
6 commits merged into from
Jul 30, 2021

Conversation

fanyang-mono
Copy link
Member

@fanyang-mono fanyang-mono commented Jul 14, 2021

Fixes #55669

@ghost
Copy link

ghost commented Jul 14, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: fanyang-mono
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

@ghost
Copy link

ghost commented Jul 14, 2021

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details
Author: fanyang-mono
Assignees: -
Labels:

area-Infrastructure

Milestone: -

@fanyang-mono
Copy link
Member Author

This change makes FreeBSD build failing though. Need to investigate further.

@ViktorHofer
Copy link
Member

cc @am11

@am11
Copy link
Member

am11 commented Jul 22, 2021

Strange that it didn't showed up in CI runs. Was it excluded for some other reason?
PackageRID is the RID to restore package. OutputRid is the RID to create shipping package.

If changing package RID is fixing the "Building Targeting Pack" issue on Android, while all other platforms are fine, I think we should find the reason why Android with CrossBuild=true is expecting package restore RID to be same as target, rather than host?

@am11
Copy link
Member

am11 commented Jul 22, 2021

Does deleting this override line fix Android test build:

<MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>
?

@fanyang-mono
Copy link
Member Author

Does deleting this override line fix Android test build:

<MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>

?

No the failure happened before reaching to this point.

@fanyang-mono
Copy link
Member Author

Strange that it didn't showed up in CI runs. Was it excluded for some other reason?
PackageRID is the RID to restore package. OutputRid is the RID to create shipping package.

If changing package RID is fixing the "Building Targeting Pack" issue on Android, while all other platforms are fine, I think we should find the reason why Android with CrossBuild=true is expecting package restore RID to be same as target, rather than host?

The Android arm64 lane only run on rolling build. That's why it didn't fail for the PR checks.

@am11
Copy link
Member

am11 commented Jul 23, 2021

The Android arm64 lane only run on rolling build. That's why it didn't fail for the PR checks.

Is rolling build different than the official build? We did ran official build in that PR.

No

Alright, then this particular leg of Android is different than anything else we have in PR, official and unofficial platform matrix. We are not passing -cross and explicitly setting /CrossBuild=false, when it is clearly cross-compilation on Ubuntu for Android. I think we should try to fix this leg's configuration, rather than changing the meaning of things again.

@fanyang-mono
Copy link
Member Author

Is rolling build different than the official build? We did ran official build in that PR.
Rolling build is the build against main, which ran a couple times per day. It is not triggered as often as for every PR.

Alright, then this particular leg of Android is different than anything else we have in PR, official and unofficial platform matrix. We are not passing -cross and explicitly setting /CrossBuild=false, when it is clearly cross-compilation on Ubuntu for Android. I think we should try to fix this leg's configuration, rather than changing the meaning of things again.

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true. I updated the condition for _packageRID to accommodate this scenario.

@ViktorHofer
Copy link
Member

Is rolling build different than the official build? We did ran official build in that PR.

Unfortunately yes but fortunately they are public builds. Rolling builds are ones that are triggered when a PR is merged, they run on the main branch. Because of the low number of hardware available for certain configurations we can't run all configurations per PR which is why some only run on rolling builds. We usually only have about 10-15 rolling builds per day (as those are batched together as well).

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true

Why is PortableBuild=true passed in at all? It's already true by default:

<PortableBuild Condition="'$(PortableBuild)' == ''">true</PortableBuild>

@fanyang-mono
Copy link
Member Author

fanyang-mono commented Jul 23, 2021

Why is PortableBuild=true passed in at all?

It was set at
https://github.com/dotnet/runtime/blob/main/src/tests/build.sh#L362
and
https://github.com/dotnet/runtime/blob/main/src/tests/build.sh#L400

The runtime test infrastructure is a complicated beast. Actually, I don't know exactly why it PortableBuild=true needs to be passed in explicitly. Maybe we don't need to. Even thought it was not passed in explicitly, PortableBuild will still be true. And the problem that this PR is trying to solve will still be there.

@am11
Copy link
Member

am11 commented Jul 23, 2021

I just did some more investigation and found that for the android arm64 runtime test build, both CrossBuild and PortableBuild were set to true. I updated the condition for _packageRID to accommodate this scenario.

Ah, ok. I was looking at (https://dev.azure.com/dnceng/public/_build/results?buildId=1237622&view=logs&j=1fbc174f-133f-57a8-dcec-0de15ad033b4&t=827c0caa-5f6d-5c33-e064-1c275c207b92) Build Product step and there we have:

/__w/1/s/build.sh -ci -arch arm64 -os Android -s mono+libs -c Release /p:CrossBuild=false

I don't know how to explain it based on what we understood. If we are cross compiling then /p:CrossBuild should be true (or just pass -cross) and ROOTFS_DIR environment variable should be set. Similarly, in test build step:

/__w/1/s/src/tests/build.sh /p:LibrariesConfiguration=Release -ci -excludemonofailures os Android arm64 /p:RuntimeVariant= Release

should have -cross flag set rather than setting it somewhere in the middle of the build? Then we won't be needing one-off workarounds like

<MicrosoftNetCoreAppRuntimePackDir>$(ArtifactsBinDir)microsoft.netcore.app.runtime.android-$(TargetArchitecture)\$(Configuration)\runtimes\android-$(TargetArchitecture)\</MicrosoftNetCoreAppRuntimePackDir>

if something is not working with ROOTFS_DIR and -cross (which has been the cross build way in runtime/coreclr/corefx/core-setup repos for a while now), we can investigate it and fix to make it work.

Copy link
Member

@am11 am11 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Looks great. :)

Android cross-build seems a bit different than other platforms, as while it is a cross build, we don't set -cross arguments (and set ROOTFS_DIR environment variable) for it. It is probably to workaround something, but IMHO, we should try to fix the reason of workaround at some point, so we could cross-build mono as well as coreclr for Android alike (#32800).

src/tests/build.sh Outdated Show resolved Hide resolved
Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@fanyang-mono
Copy link
Member Author

Thank you! Looks great. :)

Android cross-build seems a bit different than other platforms, as while it is a cross build, we don't set -cross arguments (and set ROOTFS_DIR environment variable) for it. It is probably to workaround something, but IMHO, we should try to fix the reason of workaround at some point, so we could cross-build mono as well as coreclr for Android alike (#32800).

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

@am11
Copy link
Member

am11 commented Jul 29, 2021

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

You have added an exception in test build script which reads "do not consider Android's cross build as typical cross-build".
So whatever workaround'ish reason it entails is what I was referring to. -cross is neither passed to Android's product build nor test build, while it's clearly a cross-build.

@fanyang-mono
Copy link
Member Author

Do you mean the build for Mono Android product itself or the runtime test build script that I am fixing here?

You have added an exception in test build script which reads "do not consider Android's cross build as typical cross-build".
So whatever workaround'ish reason it entails is what I was referring to. -cross is neither passed to Android's product build nor test build, while it's clearly a cross-build.

Ah, gotcha!

@fanyang-mono fanyang-mono changed the title Fix _packageRID Do not set CrossBuild to true when building runtime tests for Android Jul 29, 2021
@fanyang-mono
Copy link
Member Author

@am11 If this change looks good to you, do you mind approving it?

@am11
Copy link
Member

am11 commented Jul 30, 2021

@ViktorHofer if you could give a green light on this too that would be great. :)

@ghost
Copy link

ghost commented Jul 30, 2021

Hello @fanyang-mono!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 6fb0d44 into dotnet:main Jul 30, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 29, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

_packageRID has wrong value for runtime tests build for Android Arm64 and Arm32
3 participants